Files that changed from the base of the PR and between 922a9b5 and b09d49c.

• indexer/services/comlink/tests/lib/helpers.test.ts (5 hunks)
• indexer/services/comlink/src/controllers/api/v4/historical-pnl-controller.ts (2 hunks)
• indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3 hunks)
• indexer/services/comlink/src/lib/helpers.ts (2 hunks)

indexer/services/comlink/src/controllers/api/v4/historical-pnl-controller.ts (5)

22-22: LGTM: Import statement updated correctly.

The import statement has been updated to include aggregateHourlyPnlTicks instead of aggregatePnlTicks, which aligns with the PR objective of modifying the PnL aggregation method.

---

159-159: LGTM: Aggregation function updated to group by hour.

The change from aggregatePnlTicks to aggregateHourlyPnlTicks aligns with the PR objective of grouping PnL ticks by hour instead of by block. This should address the issue of inaccurate PnL aggregation due to ticks being generated at different block heights within the same hour.

---

159-159: Verify performance impact of new data structure.

The change from Map<number, PnlTicksFromDatabase> to PnlTicksFromDatabase[] represents a shift in the data structure. While this change is necessary for the new aggregation method, it's important to verify that it doesn't negatively impact performance or memory usage, especially for large datasets.

Could you provide some benchmarks or performance tests comparing the old and new implementations, particularly for scenarios with a large number of PnL ticks?

---

Line range hint 162-166: LGTM: historicalPnl construction updated correctly.

The historicalPnl construction has been appropriately updated to directly map over the aggregatedPnlTicks array. This change is consistent with the new data structure and should maintain the correct functionality.

---

Line range hint 1-266: Summary: Changes address the PnL aggregation issue, but require further verification.

The modifications to the getHistoricalPnlForParentSubaccount method successfully implement the new hourly PnL aggregation, addressing the issue of inaccurate aggregation due to block-based grouping. The changes are well-implemented and consistent with the PR objectives.

However, to ensure the robustness and efficiency of these changes, please address the following points:

1. Provide benchmarks or performance tests comparing the old and new implementations, especially for large datasets, to verify that the change in data structure (from Map to Array) doesn't negatively impact performance or memory usage.

2. Implement comprehensive unit tests to ensure the new aggregation method works as expected across various scenarios, including edge cases.

3. Update the relevant documentation to reflect the changes in PnL aggregation methodology.

Could you please provide information on the test plan implementation and any performance comparisons you've conducted?

indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)

96-96: Updated to use hourly PnL aggregation

Great job updating the code to use aggregateHourlyPnlTicks for hourly aggregation of PnL ticks. This aligns well with the PR objective to aggregate PnL ticks by hour instead of by block.

indexer/services/comlink/src/lib/helpers.ts (1)

687-687: Ensure consistent time zone handling when truncating timestamps.

Using DateTime.fromISO(pnlTick.createdAt).startOf('hour').toISO() relies on the default time zone, which may lead to inconsistent aggregation if createdAt timestamps are not in UTC. Explicitly specifying the time zone can prevent potential issues.

Run the following script to check if all createdAt timestamps are in UTC:

✅ Verification successful

Consistent UTC time zone handling verified for createdAt timestamps.

All createdAt fields are in UTC, ensuring consistent aggregation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all `createdAt` timestamps in PnL ticks are in UTC.

# Test: Search for `createdAt` values with time zone offsets. Expect: No matches if all are in UTC.
rg --type js --no-heading --no-filename 'createdAt:\s*"[^"]+"' | grep -E '\+\d{2}:\d{2}|\-\d{2}:\d{2}'

Length of output: 104

---

Script:

#!/bin/bash
# Description: Verify that all `createdAt` timestamps in PnL ticks are in UTC by checking for 'Z' suffix.

# Test: Search for `createdAt` values not ending with 'Z'. Expect: No matches if all are in UTC.
rg --type js 'createdAt:\s*"[^"]+[^Z]"'

Length of output: 41

---

Script:

#!/bin/bash
# Description: Check for `createdAt` fields without any time zone information.

# Test: Search for `createdAt` values without time zone offset or 'Z'. Expect: Minimal or no matches.
rg --type js 'createdAt:\s*"[^"]+(?<!Z)"'

Length of output: 293

indexer/services/comlink/__tests__/lib/helpers.test.ts (2)

47-47: Import of 'aggregateHourlyPnlTicks' is appropriate

The addition of aggregateHourlyPnlTicks to the import statement is necessary for the new test cases.

---

63-63: Importing 'DateTime' from 'luxon' is necessary

The import of DateTime from the luxon library correctly facilitates date and time manipulations in the test cases.